Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Link UI improvements #4909

Closed
wants to merge 5 commits into from
Closed

Link UI improvements #4909

wants to merge 5 commits into from

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented Feb 7, 2018

🚧 Work in progress! 🚧

Fixes #4572.

link

Still to do:

  • Padding and margin tweaks—get things looking like they're described in Iterations on link interface #4572
  • Remove 'edit link' button in lieu of just displaying a text field
  • Remove placeholder links when selection changes

@noisysocks noisysocks added the [Status] In Progress Tracking issues with work in progress label Feb 7, 2018
Insert a placeholder <a> so that it is clear which text will be made
into a link when creating a link using the RichText component.
Consolidate the dedicated 'remove link' button into the toolbar button
that adds a link.
When editing or adding a link, ESCAPE should cancel editing mode and
ENTER should submit the changes.
The link popup should widen to fit the URL within it.
Remove the placeholder link inserted when a new link is added when the
user moves the selection elsewhere.
@noisysocks
Copy link
Member Author

noisysocks commented Feb 9, 2018

I'm AFK for the next two weeks and unfortunately did not get to finish this. Happy for someone to pick it up while I'm away if they're interested.

Still to do:


The big problem that I got stuck on is having the selected text remain highlighted while the link UI is shown:

highlight

This functionality doesn't happen natively because we have configured TinyMCE to not use an iframe. This means that when the link URL input is focused, the selected text loses its highlight.

I had two ideas on how to move pass this limitation:

  1. Insert a dummy link when the 'insert link' button is clicked. This dummy link will have a magic href value of _wp_link_placeholder. When a link with a href equal to the placeholder text is selected, the link UI will treat it as a new empty link. When the user moves their cursor, any placeholder links in the editor are removed.
    This is the approach I first took, and you can see it implemented in this PR. There are two problems with it, though:
    1. Placeholder links are not always removed, e.g. if you click outside the editor
    2. Undo state is affected by adding and removing the placeholder link
  2. Fake the highlight by wrapping the selected text with a <span> that is styled to look like text selection and has a data-mce-bogus="1" attribute. The bogus attribute means that TinyMCE will not serialise the node into the HTML it outputs.
    I experimented with this approach a little but did not get very far. You can see a gist of my attempt here. The major problem I faced while taking this approach was that inserting the bogus <span> would cause TinyMCE's selection to change, which would cause the nodechange event to be triggered, which would in turn dismiss the formatting toolbar.

As my next step, I would look into adapting approach 1 to handle the selection edge case and to fix the undo state issue by using TinyMCE's ignore method. Failing that, we could, for now, abandon the highlight component of #4572 and just implement all of the other (still worthwhile) changes.

cc. @iseulde and @azaozz since they're the in-house TinyMCE experts.

@noisysocks noisysocks closed this Mar 6, 2018
@noisysocks noisysocks deleted the add/link-ui-improvements branch March 6, 2018 04:39
@noisysocks
Copy link
Member Author

Closing due to staleness. Will revisit this soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant